notify-bench: harness isolating the NOTIFY commit lock (fork-local)#84
notify-bench: harness isolating the NOTIFY commit lock (fork-local)#84NikolayS wants to merge 3 commits into
Conversation
Adds the benchmark harness, raw results and measurement instrumentation used to characterize the cluster-wide lock that PreCommit_Notify() takes. Fork-local; not proposed for upstream in this form. The central difficulty is that comparing INSERT against INSERT+NOTIFY measures the lock plus the SLRU queue insert, the utility statement and an extra round trip. The instrumentation patch resolves this by making the identical binary skip only LockSharedObject() when PG_NOTIFY_NOLOCK is set in the postmaster environment, so the two arms differ by that one call. With synchronous_commit=on and zero listeners, removing it takes throughput from 1,027 to 7,893 TPS at 16 clients and from 1,055 to 12,369 at 64 -- recovering essentially all of the gap to a NOTIFY-free workload up to 16 clients and 74% of it at 64. This shows the SLRU NotifyQueueLock does not simply become the next bottleneck. Stated without reference to this disk: the wait-event data shows both arms with one fsync in flight, but the control queues ~21 backends behind it while NOTIFY queues none, because they are stuck a step earlier. Group-commit batch factor ~21 versus exactly 1. The README documents the harness's known defects rather than hiding them, including one that invalidates the "interested listener" results and whose bias favours the conclusion the harness was built to test: psql reading from a fifo never calls PQconsumeInput, so those listeners jam in ClientWrite instead of draining. scripts/checkjam.sh demonstrates it. Those numbers are retracted; the uninterested-listener results are unaffected. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW
Every context line of a unified diff is a space followed by the original source line, which for this tree means a space followed by a tab. The stored instrumentation patch therefore trips "space before tab in indent" and "trailing whitespace" in git diff --check, which SanityCheck runs. Add a directory-local .gitattributes clearing the whitespace attribute for *.patch, mirroring what the root .gitattributes already does for other files that legitimately contain special whitespace. Scoped to this directory so the upstream root file is untouched. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW
|
CI: The failure is a single test out of 198: Two independent reasons it can't be attributable to this PR:
So this is a flaky or environment-specific ecpg failure on the Windows VS runner, not a regression. I tried to re-run the failed job immediately; the API rejected it with Generated by Claude Code |
Adds the two prototype arms measured on top of the lock-isolation result: the v1 shape (heavyweight lock, insertion before ProcArrayEndTransaction) and the v2 shape (dedicated LWLock, insertion after it), together with their harness scripts, raw CSVs and the prototype diff. v2 reaches 16,507 TPS at 64 clients against 1,055 for stock and 16,307 for a NOTIFY-free control, with commit latency falling from 60.7ms to 3.9ms. v1 reached only 10,965, so the placement and lock choice account for the remaining ~50%. The README notes what these runs cannot show: with zero listeners nothing ever reads the queue, so they cannot detect an ordering defect, and control cross-run variance at 64 clients is ~10%. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW
Fork-local infrastructure. Never intended for upstream. Everything here lives under
notify-bench/and would not be proposed to -hackers in this form. The upstream-bound patch is the documentation change in #83, which this PR supplies the supporting measurements for; background and analysis are in #82.Why it exists
The obvious experiment — compare
INSERTagainstINSERT; NOTIFY— does not isolate the lock. It measures the lock plus the SLRU queue insert, the utility statement itself, and an extra client round trip. Any speedup it shows is unattributable.notify-bench/instrumentation/0001-MEASUREMENT-ONLY-skip-notify-commit-lock.patchfixes that. It makes the identical binary skip only theLockSharedObject()call inPreCommit_Notify()whenPG_NOTIFY_NOLOCKis set in the postmaster environment. Both arms run the same executable, the same SQL, the same number of round trips, and insert the same queue entries; they differ by that one call.The skip is semantically unsafe — it breaks the commit-order guarantee that the lock exists to provide. It is a measurement device, not a proposed optimization, and the patch is named accordingly.
Headline result
synchronous_commit=on, zero listeners, 3 reps, median [min-max] TPS:INSERT)The
gap recoveredcolumn is the point. IfNotifyQueueLock— the SLRU queue lock — simply became the next bottleneck once the heavyweight lock was gone, removing the heavyweight lock would buy little. It does not: through 16 clients the no-lock arm reaches the NOTIFY-free control, and at 64 clients the residual is only ~26% of the gap. The commit-time lock, not the queue, is what bounds notifying throughput here.The hardware-independent framing
Absolute TPS on a 4-vCPU box is not interesting. The wait-event data is, because it holds regardless of how fast the disk is: both arms run with exactly one fsync in flight at all times. The difference is what is queued behind it. The control arm has ~21 backends waiting on that one flush and group-committing with it; the NOTIFY arm has zero, because those backends are stuck a step earlier, waiting for the object lock and not yet in the commit path at all.
Group-commit batch factor ~21 versus exactly 1. That ratio is a property of the serialization, not of this storage.
Known defects — documented, not hidden
notify-bench/README.mdenumerates these. The most important one invalidates a result:psql -f <fifo>listeners never callPQconsumeInput. They jam inClientWriteinstead of draining the queue.scripts/checkjam.shdemonstrates it directly: 50 of 50 listeners wedged. This flatters master and punishes v18 — that is, the bias runs in favour of the conclusion the harness was built to test, which is the worst direction for it to run. The "interested listener" results are therefore retracted. The uninterested-listener results, including the table above, do not depend on listener draining and are unaffected.Also, in decreasing order of severity:
ab_lock.csv— no repetition, no dispersion.synchronous_commit=offrows sit at roughly a 50% noise floor; differences there are not meaningful.bigserial primary key, whose sequence and index contention cap the control ceiling — in both arms, so it compresses the measured gap rather than inflating it.None of this is publication-ready. The README lists what has to be fixed before any of these numbers should be quoted outside this repository. It is checked in so the measurements behind #83 are reproducible and auditable, defects included.
🤖 Generated with Claude Code
https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW
Generated by Claude Code